perf(spanner): optimize row creation by using a shared prototype for toJSON - #9259
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes row creation in PartialResultStream by moving the toJSON method to a shared prototype instead of defining it as a per-row closure, reducing memory overhead. However, the current implementation uses Object.setPrototypeOf to mutate the prototype of newly created arrays, which is a known performance anti-pattern in V8. To avoid this penalty, it is recommended to define an ES6 class extending Array (e.g., RowImpl) and instantiate it directly instead of mutating the prototype after creation.
…toJSON Optimizes memory usage and row creation latency in PartialResultStream by eliminating per-row closure and property descriptor allocations: 1. Shared Prototype: Defines a shared prototype (`rowProto`) inheriting from `Array.prototype` with a non-enumerable `toJSON` method. 2. Hot-Path Optimization: Replaces `Object.defineProperty(fields, 'toJSON', ...)` in `_createRow` with `Object.setPrototypeOf(fields, rowProto)`. 3. Preserves Array Identity: Inheriting from `Array.prototype` ensures strict Array identity (`row.constructor === Array`, `Array.isArray(row) === true`, and `row instanceof Array === true`), preserving full compatibility with `assert.deepStrictEqual` and third-party serializers. 4. Performance Impact: Benchmarking over 500,000 rows shows ~2.1x faster row instantiation and ~26% lower retained heap (-69 MB GC churn).
1c6b0a5 to
6e5984f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes row creation in the Spanner client by introducing a RowImpl class that extends Array to share a non-enumerable toJSON method, avoiding per-row closures. However, the reviewer pointed out that subclassing Array changes the prototype of the returned rows, which introduces a breaking change for users' test suites that rely on strict deep equality checks (e.g., assert.deepStrictEqual). To resolve this while keeping the performance benefits, the reviewer suggests reverting to standard Array instantiation and instead applying a single, shared property descriptor to the array instances.
|
@olavloite changes looks good. PR description needs an update because we are not using |
Thanks for pointing that out. I did indeed change the implementation halfway, but forgot to update the description. |
2 participants
Optimizes memory usage and row creation latency in
PartialResultStreamby eliminating per-row closure and property descriptor allocations:RowImplSubclass: Defines aRowImplclass extendingArray<Field>withtoJSONimplemented directly on its prototype, avoiding per-row method allocations and eliminating any need for dynamic prototype mutation (Object.setPrototypeOf).new Array(len)followed byObject.defineProperty(fields, 'toJSON', ...)in_createRowwithnew RowImpl(len), constructing rows directly with the shared prototype method from the start.RowImpl.prototype.constructor = Arrayonce at module load ensures identical semantics to native Arrays (Array.isArray(row) === true,row instanceof Array === true, androw.constructor === Array), preserving full compatibility withassert.deepStrictEqual, JSON serialization, and external libraries.